-
Notifications
You must be signed in to change notification settings - Fork 913
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
keep the persistent connection only if rosmaster supports http1.1 #2208
keep the persistent connection only if rosmaster supports http1.1 #2208
Conversation
Signed-off-by: Chen Lihui <lihui.chen@sony.com>
@sloretz @jacobperron @mjcarroll friendly ping, requesting maintainer review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look okay to me, but this breaks ABI. I'd rather not release an ABI-breaking change into an already released distribution. Maybe there's a way to make this fix ABI-compatible?
@@ -118,6 +118,9 @@ namespace XmlRpc { | |||
// each thread should have its own client. | |||
bool _executing; | |||
|
|||
// True if the server supports HTTP1.1 | |||
bool _server_http_1_1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change breaks ABI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but this breaks ABI.
our bad, that should be kept...sorry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I knew it breaks ABI, but it's clear.
I tried to use _connectionState
, but some source code might be as
enum ClientConnectionState { NO_CONNECTION, CONNECTING, WRITE_REQUEST, READ_HEADER, READ_RESPONSE, IDLE,
CLIENT_CONNECT_STATE_MASK = 0xFFFF, SERVER_HTTP_DISCONNECT = 0x10000};
check header to set _connectionState:
_connectionState |= SERVER_HTTP_DISCONNECT;
set:
_connectionState = READ_HEADER;
-->
_connectionState |= READ_HEADER;
if (_connectionState == WRITE_REQUEST)
-->
if (_connectionState & CLIENT_CONNECT_STATE_MASK == WRITE_REQUEST)
check if SERVER_HTTP_DISCONNECT:
if (_connectionState & SERVER_HTTP_DISCONNECT)
do you think it's acceptable?
It seems to not work fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have any other suggestions? I'd like to hear that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…p1.1" This reverts commit 2cb7602. Signed-off-by: Chen Lihui <lihui.chen@sony.com>
2a3f123
to
7750cc5
Compare
Signed-off-by: Chen Lihui <lihui.chen@sony.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for the fix!
Should I take care of the Melodic backport? Or is it planned? |
I think a maintainer can include it in a backport PR, e.g. #2152 |
) * keep the persistent connection only if rosmaster supports http1.1 Signed-off-by: Chen Lihui <lihui.chen@sony.com> * Revert "keep the persistent connection only if rosmaster supports http1.1" This reverts commit 2cb7602. Signed-off-by: Chen Lihui <lihui.chen@sony.com> * fix without breaking ABI Signed-off-by: Chen Lihui <lihui.chen@sony.com> * Update comment Co-authored-by: Jacob Perron <jacob@openrobotics.org>
to fix #2182
Signed-off-by: Chen Lihui lihui.chen@sony.com